feat(react): createPortal via nodesRef patch ops#2543
Conversation
🦋 Changeset detectedLatest commit: 9821d16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
67fc30b to
94f9f0b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds createPortal support (public API and runtime exports), NodesRef serialization and RefProxy selector, two new snapshot operations with main-thread appliers, a pre-hydration pending queue and replay, an instance-tree reconstruction helper, background hydrate integration, and extensive snapshot and RTL tests. ChangesPortal Rendering and Patch Operations
Testing Infrastructure and Helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67fc30b68a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b424bce to
1565409
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
aa72c8d to
2e85cbe
Compare
React Example#7883 Bundle Size — 235.77KiB (+0.66%).9821d16(current) vs 0d51ee8 main#7874(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
React MTF Example#1013 Bundle Size — 206.69KiB (+0.76%).9821d16(current) vs 0d51ee8 main#1004(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
React External#998 Bundle Size — 690.27KiB (+0.67%).9821d16(current) vs 0d51ee8 main#989(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
Web Explorer#9455 Bundle Size — 900.02KiB (0%).9821d16(current) vs 0d51ee8 main#9446(baseline) Bundle metrics
Bundle size by type
|
| Current #9455 |
Baseline #9446 |
|
|---|---|---|
495.88KiB |
495.88KiB |
|
401.92KiB |
401.92KiB |
|
2.22KiB |
2.22KiB |
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard
Generated by RelativeCI Documentation Report issue
d9ff5c1 to
6bbc286
Compare
Renders a vnode subtree into a different ReactLynx element identified by
a `NodesRef` (from `ref={setX}` or `lynx.createSelectorQuery()`), without
requiring any compile-time marker attribute. Implementation routes portal
ops through the existing SnapshotInstance/patch abstraction:
- New `nodesRefInsertBefore` / `nodesRefRemoveChild` patch ops; carried
via the regular `LifecycleConstant.patchUpdate` channel alongside BSI
CreateElement / InsertBefore / RemoveChild ops.
- `fakeRoot.insertBefore` wires `child.__parent = fakeRoot` so preact's
`removeNode` (which walks `child.parentNode.removeChild`) routes through
portal removeChild, otherwise unmount silently no-ops.
- Pre-hydrate Portal mounts queue into `pendingInsertBefore`;
`clearPendingPortalInsertBefore` (called from hydrate) replays the BSI
subtree's dropped CreateElement / SetAttributes / internal InsertBefore
ops via `reconstructInstanceTree`, then attaches the subtree to host
via `nodesRefInsertBefore`.
- `reconstructInstanceTree` extracted to its own module so portal's
pre-hydrate replay can share the helper without forming an import
cycle with `backgroundSnapshot.ts`.
Different design from #2501 (which uses a `portal-container` SWC
transform to lift the host subtree into a separate snapshot) — this one
stays inside the existing SnapshotInstance/patch model so hydrate diff
and future first-screen-direct-render paths can be reused without
protocol changes.
Tests cover pre-/post-hydrate mount, unmount via `componentWillUnmount`,
container swap, multi-child reorder + prepend, context propagation
across portal boundary, ctx-not-found soft-fail on apply, and host
selector miss; runtime test env gets `__GetPageElement` /
`__QuerySelector` mocks. testing-library suite includes a preact-parity
case ported from internal-preact's `feat/portal-slot` branch verifying
that portal content stays put while host's normal children toggle.
6bbc286 to
ffc0600
Compare
`nodesRefInsertBefore` / `nodesRefRemoveChild` previously soft-failed when the selector didn't resolve or the BSI subtree wasn't materialized. That's a caller bug (stale `NodesRef`), so use non-null assertions and throw instead of silently dropping the op.
- Throw meaningful errors (not raw `TypeError`s) when `nodesRefInsertBefore` / `nodesRefRemoveChild` apply hits a caller-bug state — message names the op, the childId, the selector, and hints at the most likely cause. - Move the portal-only apply handlers + selector lookup into a new `nodesRefApply.ts` module so `snapshotPatchApply.ts` stays focused on snapshot-tree ops. - Reject non-CSS-selector `NodesRef`s (`selectReactRef`, `selectUniqueID`) in `serializeNodesRef` instead of silently no-oping on the main thread. - Drain matching pending inserts from `pendingInsertBefore` when a portal child is unmounted before hydrate, so the queue replay during hydrate doesn't resurrect a child that was already torn down on background. - Switch the container-swap test to assert the portaled subtree actually moves from container A to container B. - Use `toThrowErrorMatchingInlineSnapshot` for the throw-path tests.
## Summary [#2538](#2538) dropped `//#build` from `build.dependsOn` to reduce cache fanout. `//#build` runs the root `tsc --build`, which produces the composite-project `lib/` outputs that several workspace packages declare as their public types: - `@lynx-js/rspeedy` → `"types": "./lib/index.d.ts"` - `@lynx-js/template-webpack-plugin`, `@lynx-js/web-rsbuild-server-middleware`, etc. `//#build` still runs as part of `pnpm turbo build` (no filter), but now in parallel with package builds. Two packages whose `tsgo` dts generation imports types from those `lib/` outputs race against `//#build`: - `@lynx-js/config-rsbuild-plugin` (imports `@lynx-js/rspeedy`, `@lynx-js/template-webpack-plugin` in `LynxConfigWebpackPlugin.ts` / `pluginLynxConfig.ts`) - `@lynx-js/reactlynx-testing-library` (imports `@lynx-js/rspeedy` in `rstest-config.ts`) When the race is lost, those builds fail with `TS2307: Cannot find module …`. This was hit on PR #2543 ([build / Build (Ubuntu)](https://github.com/lynx-family/lynx-stack/actions/runs/25103126514/job/73557596812) — same failure on Windows). ## Fix Add `//#build` to the `dependsOn` of just the two affected packages (not to the global `build.dependsOn`) so they wait for `tsc --build` without re-introducing the cache fanout #2538 was avoiding. ## Test plan - [x] ```bash find packages -name "*.tsbuildinfo" -delete find packages -type d -name lib -exec rm -rf {} + pnpm turbo build --force ``` succeeds end-to-end (49/49 tasks). On `main` (without this fix) the same command fails on `@lynx-js/config-rsbuild-plugin#build` and `@lynx-js/reactlynx-testing-library#build`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Resolved TypeScript type resolution failures in clean CI/build environments. * **Chores** * Ensured root composite build runs before package builds to enforce correct build ordering and artifact availability. * Added build configuration for a new extractor package and updated package build dependencies. * **Documentation** * Clarified maintenance guidance for produced/generated build artifacts and cache outputs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Adds two cross-component tests in `react/testing-library` that drive the
portal end-to-end via `lynx.createSelectorQuery().select('#id')` instead
of a React ref — covers the CSS-selector `NodesRef` apply path that
real apps hit when the host and the consumer don't share a ref tree.
Also fixes the testing-library mock to match real Lynx behavior:
`select(selector)` now stores the CSS selector string in
`_nodeSelectToken.identifier` (instead of pre-resolving to the element's
unique id). `setNativeProps` and `fireEvent`'s `NodesRef` branch resolve
ID_SELECTOR tokens via `document.querySelector` and UNIQUE_ID tokens
via `__GetElementByUniqueId`, keeping existing tests passing.
`createPortal` is only invoked from the background thread — short-circuit to `null` on main thread so preact's `render`/`createElement` and the BSI linkage helpers don't get pulled into the main-thread chunk. Fixes a typo (`if (__MAIN_THREAD__) null;` was a discarded expression that left the function falling through), updates the public api-extractor baseline (`VNode<any>` → `VNode<any> | null`), and updates the runtime test to assert the early-return on main thread + materialization on background thread. Also adds `nodesRefInsertBefore` / `nodesRefRemoveChild` to the `formatPatch.test.ts` fixture so the params-metadata for those entries is regression-checked.
Drops the api-extractor `ae-missing-release-tag` warning by tagging `createPortal` `@public`, matching how it's already exposed via `react.docs.d.ts`. Refreshes the api-extractor baseline accordingly.
Merging this PR will improve performance by 19.23%
Performance Changes
Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2c5cbda4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/testing-library/testing-environment/src/index.ts (1)
448-458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't require the selector target to exist at
select()time.
select()now throws if the element is not in the document yet, but this same file resolves selector-basedNodesRefs later insetNativeProps().exec(). That makes the testing environment stricter than the modeled API and breaks valid cases where the target is created after theNodesRefis built.💡 Suggested fix
- const el = lynxTestingEnv.env.window.document.querySelector( - selector, - ) as LynxElement; - if (!el) { - throw new Error( - `[createSelectorQuery.select] No element matches selector: ${selector}`, - ); - } + try { + lynxTestingEnv.env.window.document.querySelector(selector); + } catch { + throw new Error( + `[createSelectorQuery.select] Invalid selector: ${selector}`, + ); + } return new NodesRef({}, { type: IdentifierType.ID_SELECTOR, identifier: selector, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/testing-library/testing-environment/src/index.ts` around lines 448 - 458, createSelectorQuery.select is eagerly querying the document via lynxTestingEnv.env.window.document.querySelector and throwing if no element exists, but the runtime resolves selector-based NodesRef later in setNativeProps().exec(); remove the eager existence check and instead store the selector string (as a NodesRef) even if querySelector returns null, so createSelectorQuery.select returns the selector-based reference without throwing and setNativeProps().exec() continues to resolve the selector to a LynxElement at execution time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/src/snapshot/lifecycle/patch/nodesRefApply.ts`:
- Around line 63-70: The current logic silently falls back to __AppendElement
when beforeId is provided but unresolved; change it so that when beforeId !==
undefined and snapshotInstanceManager.values.get(beforeId) is missing or lacks
__element_root you treat this as an error instead of appending: locate the block
that checks beforeId, snapshotInstanceManager.values.get(beforeId), and calls
__InsertElementBefore or __AppendElement, and replace the silent-append behavior
with error handling (e.g., throw or processLogger.error + throw) that surfaces
the unresolved beforeId (include the beforeId and any relevant context like
host/childRoot) so stale patch bugs aren’t masked; only call __AppendElement
when beforeId === undefined.
In `@packages/react/runtime/src/snapshot/lynx/nodesRef.ts`:
- Around line 35-46: The code force-casts nodesRef to access _nodeSelectToken
and then reads nodeSelectToken.type unguarded; instead, first check that
(nodesRef as any)._nodeSelectToken exists and is an object (e.g. validate its
presence and that it has a type field) before dereferencing, and if not present
or invalid throw the same explicit Error used for unsupported NodeSelectType;
update the logic around the nodeSelectToken extraction in nodesRef handling
(symbols: nodesRef, _nodeSelectToken, NodeSelectToken, NodeSelectType,
createPortal) so malformed inputs produce the clear `[createPortal] unsupported
NodesRef type ...` error rather than an opaque TypeError.
In `@packages/react/runtime/src/snapshot/lynx/portals.ts`:
- Around line 106-131: The portal removeChild implementation only detaches the
node and cancels pendingInsertBefore but must mirror the full teardown performed
by BackgroundSnapshotInstance.removeChild: update the portal removeChild (the
function in portals.ts) to mark the removed subtree as removed, clear/null any
descendant snapshot refs, and enqueue the subtree's background snapshot id into
globalBackgroundSnapshotInstancesToRemove (using the same mechanism
backgroundSnapshotInstanceManager/BackgroundSnapshotInstance.removeChild uses)
before returning; keep the existing pendingInsertBefore splice and the
nodesRefRemoveChild patch emission but add the descendant-ref clearing and
instance-queueing steps so portaled unmounts properly clean up
backgroundSnapshotInstanceManager.
In `@packages/testing-library/testing-environment/src/lynx/ElementPAPI.ts`:
- Around line 494-508: The finally currently flips back to the background thread
immediately even if componentAtIndex returns a Promise; change the logic in the
componentAtIndex call site so you call
globalThis.lynxTestingEnv.switchToMainThread(), invoke componentAtIndex(e,
$$uiSign, index, ...args) and capture the result, then if the result is a
Promise attach .finally(() => { if (isBackground)
globalThis.lynxTestingEnv.switchToBackgroundThread(); }) to restore the thread
after the async continuation; if the result is not a Promise restore immediately
as before. Ensure you reference componentAtIndex, $$uiSign, isBackground,
switchToMainThread and switchToBackgroundThread when making this change.
---
Outside diff comments:
In `@packages/testing-library/testing-environment/src/index.ts`:
- Around line 448-458: createSelectorQuery.select is eagerly querying the
document via lynxTestingEnv.env.window.document.querySelector and throwing if no
element exists, but the runtime resolves selector-based NodesRef later in
setNativeProps().exec(); remove the eager existence check and instead store the
selector string (as a NodesRef) even if querySelector returns null, so
createSelectorQuery.select returns the selector-based reference without throwing
and setNativeProps().exec() continues to resolve the selector to a LynxElement
at execution time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b66d4281-9eb1-4c4c-8ab9-759e97f15ec9
📒 Files selected for processing (25)
.changeset/feat-react-portal-patch-channel.mdpackages/react/etc/react.api.mdpackages/react/runtime/__test__/snapshot/debug/formatPatch.test.tspackages/react/runtime/__test__/snapshot/lynx/portals.test.jsxpackages/react/runtime/__test__/snapshot/utils/nativeMethod.tspackages/react/runtime/lazy/compat.jspackages/react/runtime/lazy/react.jspackages/react/runtime/src/index.tspackages/react/runtime/src/snapshot/lifecycle/patch/nodesRefApply.tspackages/react/runtime/src/snapshot/lifecycle/patch/snapshotPatch.tspackages/react/runtime/src/snapshot/lifecycle/patch/snapshotPatchApply.tspackages/react/runtime/src/snapshot/lifecycle/ref/delay.tspackages/react/runtime/src/snapshot/lynx/nodesRef.tspackages/react/runtime/src/snapshot/lynx/portals.tspackages/react/runtime/src/snapshot/lynx/portalsPending.tspackages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/snapshot/reconstructInstanceTree.tspackages/react/runtime/types/types.d.tspackages/react/testing-library/src/__tests__/list.test.jsxpackages/react/testing-library/src/__tests__/portals.test.jsxpackages/react/testing-library/src/fire-event.tspackages/react/types/react.docs.d.tspackages/testing-library/testing-environment/etc/testing-environment.api.mdpackages/testing-library/testing-environment/src/index.tspackages/testing-library/testing-environment/src/lynx/ElementPAPI.ts
The patch buffer is JSON-roundtripped between threads, which converts the `undefined` slot emitted by `before?.__id` into `null`. The apply side previously checked `beforeId !== undefined` and silently fell back to `__AppendElement` when the lookup missed, which masked the wrong ordering on `__InsertElementBefore` paths. Switch to `beforeId != null` and assert insertion order in the prepend test so the regression to the append fallback can't slip back in.
`applyNodesRefRemoveChild` previously detached the portaled element and dropped its `SnapshotInstance` entries from the manager, but never ran the recursive `unref` step that `SnapshotInstance.removeChild` performs (snapshot.ts:431). As a result, `main-thread:ref` callbacks on portaled subtrees never invoked the cleanup function returned at mount time, and `WorkletRefImpl`s under the portal kept pointing at the removed element. Mirror the regular teardown by calling `unref(child, true)` before `__RemoveElement`. Adds a testing-library reproducer that asserts a `main-thread:ref` callback's returned cleanup runs on portal unmount.
Unknown `childId` in `nodesRefInsertBefore` / `nodesRefRemoveChild` is a bg/main desync (stale background reference, double-unmount, etc.) — surface it via `sendCtxNotFoundEventToBackground` so the background can re-sync, mirroring the regular `InsertBefore` / `RemoveChild` ops in the same dispatcher instead of throwing. The `applyNodesRef*` functions now take a resolved `SnapshotInstance`, so the dispatcher does the lookup and routing. Host-resolution failures on insert still throw (caller-side input error, not a patch-channel desync).
…ch-channel # Conflicts: # packages/react/runtime/src/index.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78017dc08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
React Example with Element Template#148 Bundle Size — 198.57KiB (0%).9821d16(current) vs 0d51ee8 main#139(baseline) Bundle metrics
Bundle size by type
|
| Current #148 |
Baseline #139 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
52.81KiB |
52.81KiB |
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard
Generated by RelativeCI Documentation Report issue
`applyNodesRefRemoveChild` previously only deleted SI entries from the manager. Mirror the full teardown that `SnapshotInstance.removeChild` runs: - `snapshotDestroyList(v)` for `<list>` holders, otherwise native list callbacks + `gSignMap`/`gRecycleMap` keep stale state across portal mount/unmount cycles. - Null out `__parent` / `__previousSibling` / `__nextSibling`. - Drop `__elements` / `__element_root` refs. Keeps the portal teardown at parity with the regular tree teardown so future cleanup added to the regular path won't silently miss portaled subtrees.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37cdfbc0d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/runtime/src/snapshot/lifecycle/patch/nodesRefApply.ts`:
- Around line 101-103: The code is directly mutating private SnapshotInstance
fields (__parent, __previousSibling, __nextSibling) from outside the class which
breaks strict TypeScript; add a public instance method (e.g.,
SnapshotInstance.clearLinks() or SnapshotInstance.unlink()) or a module-scoped
helper inside the SnapshotInstance declaring module that sets those three
private fields to null, keep it exported/usable, and then replace the direct
assignments in nodesRefApply (the places that currently do v.__parent = null;
v.__previousSibling = null; v.__nextSibling = null;) with a call to that new
method (e.g., v.clearLinks()) so the private members are only mutated inside the
class/module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a28c908-4f93-4b24-80e0-fa3a9b92e52d
📒 Files selected for processing (1)
packages/react/runtime/src/snapshot/lifecycle/patch/nodesRefApply.ts
Reproducer for the snapshotDestroyList gap fixed in 37cdfbc: spy on `__UpdateListCallbacks`, render a portal containing a `<list>`, unmount it, and assert an additional `__UpdateListCallbacks` call (the cleanup trio that snapshotDestroyList emits) fires. Verified the test fails on the previous commit (`27da0f68`, no list teardown) and passes on `37cdfbc0` (with the fix).
`__parent` / `__previousSibling` / `__nextSibling` are `private` on
`SnapshotInstance`; the previous commit assigned them directly which
broke `tsc --build`. Wrap in the same `as unknown as { ... }` cast that
`portals.ts` already uses for `__parent`. Local vitest didn't catch it
because vitest doesn't type-check; CI's `tsc --build` step did.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99d9b41f0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/testing-library/src/__tests__/portals.test.jsx`:
- Line 4: Add a global test teardown to restore spies/mocks after each test to
prevent accumulated vi.spyOn() call history from breaking assertions that rely
on fixed call indices or counts; in the test file's top-level scope (the module
that declares the tests and uses vi.spyOn), add afterEach(() => {
vi.restoreAllMocks(); }) so spies created by vi.spyOn() are reset between tests
and deterministic assertions at locations referencing call indices/counts will
not fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43726fa3-b060-466f-880c-b72270374894
📒 Files selected for processing (2)
packages/react/runtime/src/snapshot/lifecycle/patch/nodesRefApply.tspackages/react/testing-library/src/__tests__/portals.test.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/runtime/src/snapshot/lifecycle/patch/nodesRefApply.ts
The runtime coverage threshold is 100%. The new `snapshotDestroyList` branch in `applyNodesRefRemoveChild` (37cdfbc) was only exercised by the testing-library suite, which doesn't feed runtime coverage — so test-react/Vitest runners both failed at the threshold check. Add a runtime-level unit test that builds a list-holder SI directly via `__SNAPSHOT__(<list>{HOLE}</list>)`, runs `nodesRefRemoveChild`, and asserts `snapshotDestroyList`'s observable side effects (cleanup trio replaces the real list callbacks, manager entry deleted).
`fakeRoot.removeChild` previously emitted only the main-thread `nodesRefRemoveChild` op, leaving the portaled `BackgroundSnapshotInstance` subtree alive in `backgroundSnapshotInstanceManager`. Repeated mount/unmount cycles accumulated stale BSIs. Mirror `BackgroundSnapshotInstance.removeChild`'s bookkeeping: mark `__removed_from_tree` and push the child id into `globalBackgroundSnapshotInstancesToRemove` so commit captures it for the debounced 10s `tearDown` (which traverses the subtree and drops entries from the manager). The pre-existing e2e test asserted `bg manager size === 0` post-unmount while importing the manager from `lib/` — a separate, stale module copy. Switch to `src/` and account for the bg root SI (size === 1 baseline). Add a new state-change-unmount reproducer that uses fake timers to drive the debounced cleanup.
Summary
Adds
createPortalto@lynx-js/react. Renders a vnode subtree into a different ReactLynx element identified by aNodesRef(fromref={setX}orlynx.createSelectorQuery()), with no compile-time marker required and arbitrary host structure permitted.Different design vs #2501
This is an alternative implementation to #2501; both target the same feature with substantially different architectures. Brief comparison:
portal-containerattr requiredLifecycleConstant.patchUpdatechannelreconstructInstanceTreeConcretely, this implementation routes everything through
SnapshotInstanceso no new lifecycle protocol is introduced; the trade-off is that pre-hydrate Portal mounts need an extra queue-and-replay step (pendingInsertBefore→clearPendingPortalInsertBefore) instead of being lifted by a transform.Limitation
Currently
NodesRefis a BTS type, we cannot get it on MTS. So IFR is not supported.Implementation notes
nodesRefInsertBefore(identifier, childId, beforeId?)/nodesRefRemoveChild(identifier, childId). Carried via the existingpatchUpdatechannel alongside BSICreateElement/InsertBefore/RemoveChildops; no new protocol.fakeRoot.insertBeforewireschild.__parent = fakeRootso preact'sremoveNode(which walkschild.parentNode.removeChild(child)) routes through portalremoveChild. Without this, unmount silently no-ops.CreateElementpush is dropped (global buffer isundefined), sofakeRoot.insertBeforequeues intopendingInsertBefore.clearPendingPortalInsertBefore(called fromhydrate()) replays the dropped subtree ops viareconstructInstanceTree([child]), then emitsnodesRefInsertBeforeto attach to host.reconstructInstanceTreeextracted to its own module (snapshot/reconstructInstanceTree.ts) so portal pre-hydrate replay can share the helper without forming an import cycle withbackgroundSnapshot.ts.Test coverage
Two test suites:
runtime/__test__/snapshot/lynx/portals.test.jsx— unit tests on the runtime path (10 cases):createPortalreturns a VNode whosecontainerInfopoints at the host_this._container !== containerpath)before?.__idtruthy branch + apply__InsertElementBefore)ContextProviderwrapper)serializeNodesRefnon-RefProxy pathnodesRefInsertBefore/nodesRefRemoveChildctx-not-found soft-failtesting-library/src/__tests__/portals.test.jsx— high-level scenarios mirroring PR #2501's portal.test.jsx (16 cases): basic render, re-render on portalled state change, context forwarding, event-bubbling semantics across portal boundary, third-party-slot pattern, mount/unmount toggle, cleanup on unmount, host swap.Runtime test env (
__test__/snapshot/utils/nativeMethod.ts) gets__GetPageElement+__QuerySelectormocks ([attr]/[attr=value]selectors).Coverage: 100% lines / 100% branches / 100% functions / 100% statements on the runtime suite.
Test plan
pnpm -F @lynx-js/react-runtime test— 570 passing, 100% coveragepnpm -F @lynx-js/reactlynx-testing-library test:base -- src/__tests__/portals.test.jsx— 16 passingSummary by CodeRabbit
New Features
Tests
Improvements